-
Notifications
You must be signed in to change notification settings - Fork 474
feat(Type Cache Deprecation): LookUp Child changes #4290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4290 +/- ##
==========================================
+ Coverage 83.38% 83.47% +0.09%
==========================================
Files 162 162
Lines 19426 19469 +43
==========================================
+ Hits 16198 16252 +54
+ Misses 2628 2617 -11
Partials 600 600
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces changes related to the deprecation of the type cache, primarily by adding a new configuration flag EnableTypeCacheDeprecation and modifying lookup functions to conditionally use this flag. The changes involve passing this flag down to various GCS operations and introducing a metadataCacheTtlSecs field for future caching logic. New test cases have been added to cover the deprecated type cache path for files and directories. However, there are several areas that require attention, including inconsistencies in passing fetchOnlyFromCache to helper functions, incorrect error handling for cache misses in cache-only lookups, a missing lookup path for implicit directories when the type cache is deprecated, and redeclaration issues for err and result variables. Additionally, the ListPrefix utility hardcodes the IsTypeCacheDeprecated flag to false, which might lead to unintended behavior. The new tests also lack coverage for cache-only lookup scenarios and CacheMissError handling.
aba7a2a to
27fde0e
Compare
Summary of ChangesHello @Tulsishah, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the object type lookup mechanism by deprecating the dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively implements the deprecation of typeCache by integrating its functionality into the statCache, controlled by the EnableTypeCacheDeprecation feature flag. The changes in LookUpChild to support a two-phase lookup (cache-first, then GCS fallback) are well-implemented. The accompanying tests are comprehensive and cover various scenarios, including cache hits, cache misses, and different object types for both hierarchical and non-hierarchical buckets.
My main feedback is to refactor a duplicated block of code in LookUpChild to improve maintainability. Overall, this is a solid contribution towards unifying the caching logic.
|
Hi @vadlakondaswetha, @ashmeenkaur, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
1 similar comment
|
Hi @vadlakondaswetha, @ashmeenkaur, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
4b213d5 to
8d43913
Compare
| MaxResults: 1, | ||
| Prefix: name.GcsObjectName(), | ||
| MaxResults: 1, | ||
| IsTypeCacheDeprecated: isTypeCacheDeprecated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is typeCacheDeprecated flag being passed as part of listObjects request>?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is to enable new caching logic in fast stat bucket
ref:
| if req.IsTypeCacheDeprecated { |
| Prefix: d.Name().GcsObjectName(), | ||
| ContinuationToken: tok, | ||
| MaxResults: limit + 1, // to exclude itself | ||
| IsTypeCacheDeprecated: d.isEnableTypeCacheDeprecation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. we should not pass flag values as part of request object for every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this then with list call caching will happen correctly with type cache deprecation otherwise implicit directories will not get cached correctly.
d48c836 to
3871e32
Compare
3871e32 to
e137a4c
Compare
Description
This PR updates the LookUpChild logic to support the deprecation of the typeCache by merging its functionality into a single, unified statCache.
Key Updates:
Link to the issue in case of a bug fix.
b/476918868
Testing details
Any backward incompatible change? If so, please explain.